Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

e2e: add pod/container info in error form exec in pod. #2091

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

tommyp1ckles
Copy link
Contributor

@tommyp1ckles tommyp1ckles commented Nov 6, 2023

When running connectivity tests, it's up to debug logs to provide information on failure errors. This is not always the case, making it hard to understand where to start investigating errors.

This adds pod/container to the error message for calls to k8s exec to make it easier to understand where the issue occurred.

Ex use case:

Running test echo-ingress-l7-named-port: setting up test: applying network policies: policies were not applied on all Cilium nodes in time: Timeout occurred

Raises the questions:

  • On what Agent Pod?
  • "Timeout occurred" error is ambiguous, did the k8s exec request timeout, or the passed command itself?

@tommyp1ckles
Copy link
Contributor Author

/test

@tklauser
Copy link
Member

Closed and re-opened PR to retrigger all tests. They were failing because of the GH incident last week.

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failure looks legitimate

k8s/exec.go Outdated Show resolved Hide resolved
@brb
Copy link
Member

brb commented Nov 22, 2023

@tommyp1ckles Could we get this PR? 🙏

When running connectivity tests, it's up to debug logs to provide information on failure errors.
This is not always the case, making it hard to understand where to start investigating errors.

This adds pod/container to the error message for calls to k8s exec to make it easier to understand where the issue occured.

Signed-off-by: Tom Hadlaw <[email protected]>
@tommyp1ckles
Copy link
Contributor Author

woops, yup last push should fix failures @tklauser @brb

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 23, 2023
@tklauser tklauser merged commit 7ba9ec4 into main Nov 23, 2023
19 checks passed
@tklauser tklauser deleted the pr/tp/improve-exec-errors branch November 23, 2023 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants